Skip to content

Add dialog to save config#2100

Merged
balloob merged 10 commits intohome-assistant:devfrom
bramkragten:take-control
Nov 25, 2018
Merged

Add dialog to save config#2100
balloob merged 10 commits intohome-assistant:devfrom
bramkragten:take-control

Conversation

@bramkragten
Copy link
Copy Markdown
Member

@bramkragten bramkragten commented Nov 23, 2018

image

Relies on home-assistant/core#18655

Closes #2092

@ghost ghost assigned bramkragten Nov 23, 2018
@ghost ghost added the in progress label Nov 23, 2018
import { LovelaceCardConfig } from "../types";
import { LovelaceConfig, LovelaceCardConfig } from "../types";

export const migrateConfig = (hass: HomeAssistant): Promise<void> =>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future PR, we should move this file to src/data/lovelace.ts, as that's also what we do for all other components.

import { html, LitElement, PropertyDeclarations } from "@polymer/lit-element";
import { fireEvent } from "../../../common/dom/fire_event";
import {
showEditCardDialog,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect, we should do this for all dialogs.

cardConfig: this.cardConfig,
reloadLovelace: () => fireEvent(this, "config-refresh"),
});
showEditCardDialog(this, this.hass!, this.cardConfig!, () =>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should take 2 arguments; this and an object with type EditCardDialogParams. It allows type checking and allows us to just store 1 object in the dialog class too. Example

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you already have this with EditDialogParams. Well then this function just needs an object of type EditDialogParams passed in .


export const showEditCardDialog = (
element: HTMLElement,
hass: HomeAssistant,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to pass hass, the dialog manager already takes care of it .

this._cardConfig = cardConfig;
this._reloadLovelace = reloadLovelace;
public async showDialog(params: EditDialogParams): Promise<void> {
this._hass = params.hass;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just this._params = params

});

export class HuiDialogEditCard extends LitElement {
protected _hass?: HomeAssistant;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


export const showSaveDialog = (
element: HTMLElement,
hass: HomeAssistant,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to pass hass, we should pass a single object to showSaveDialog

cardConfig: this.cardConfig!,
reloadLovelace: () => fireEvent(this, "config-refresh"),
};
showEditCardDialog(this, editCardDialogParams);
Copy link
Copy Markdown
Member

@balloob balloob Nov 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to just do showEditCardDialog(this, { … }). TypeScript will look at the shape of an object, not the "type" (if it's not a class)

`;
if (!this._params) {
return html``;
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the else as you return inside the if, that will keep the function a bit more readable.

if (!this._params) {
return html``;
} else {
return html`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the whole tag is just 1 turnery statement, I would split it up:

if (!this.params.cardConfig!.id) {
  return html`
    <hui-migrate-config
  `
}

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few teeny tiny comments, ok to merge after that.

@balloob balloob merged commit 69df617 into home-assistant:dev Nov 25, 2018
@ghost ghost removed the in progress label Nov 25, 2018
@balloob balloob mentioned this pull request Dec 5, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow migrating from auto LL config to manual config

3 participants